Skip to content

Conversation

crgwbr
Copy link
Contributor

@crgwbr crgwbr commented Dec 20, 2024

  • Fixes several exceptions that'd be thrown when trying to use Python sets as config values, rather than tuples or lists.
  • Removes duplicate values from directives when building the full CSP header.

@jwhitlock
Copy link
Member

In case the CI test results are private, the error for Python 3.13 is:

  ___________________________ test_default_src_is_set ____________________________
  csp/tests/test_utils.py:62: in test_default_src_is_set
      policy_eq("default-src example.com example2.com", policy)
  csp/tests/test_utils.py:11: in policy_eq
      assert parts_a == parts_b, f"{a!r} != {b!r}"
  E   AssertionError: 'default-src example.com example2.com' != 'default-src example2.com example.com'
  E   assert ['default-src...example2.com'] == ['default-src... example.com']
  E     
  E     At index 0 diff: 'default-src example.com example2.com' != 'default-src example2.com example.com'
  E     
  E     Full diff:
  E       [
  E     -     'default-src example2.com example.com',
  E     +     'default-src example.com example2.com',
  E       ]

@jwhitlock
Copy link
Member

After some thought, I'm slightly negative on this PR. Looking at the Django settings reference, I can't find any settings that use a set but instead use a list and default to an empty list []. Looking further, there is code that raises an exception if certain settings are not a list or tuple. This matters because our goal is to get some version of this code into Django. If they have a policy of non converting set settings values, maybe we should as well, and leave it to the user to convert them as they'd like (list(set_value), or even list(sorted(set_value)))

@crgwbr
Copy link
Contributor Author

crgwbr commented Jan 9, 2025

After some thought, I'm slightly negative on this PR. Looking at the Django settings reference, I can't find any settings that use a set but instead use a list and default to an empty list []. Looking further, there is code that raises an exception if certain settings are not a list or tuple. This matters because our goal is to get some version of this code into Django. If they have a policy of non converting set settings values, maybe we should as well, and leave it to the user to convert them as they'd like (list(set_value), or even list(sorted(set_value)))

To me, it seems unpythonic to force the value to be a list or tuple, when really all we need is any Collection[str]. It'd only make sense to me to constrain it more than that in cases where ordering is significant—like Django's middleware or installed apps settings. In this case, CSP rules are by-spec not ordered. And sending duplicate values (what using sets aims to prevent) does have a real user impact due to wasting bandwidth.

@jwhitlock
Copy link
Member

To me, it seems unpythonic to force the value to be a list or tuple, when really all we need is any Collection[str]. It'd only make sense to me to constrain it more than that in cases where ordering is significant—like Django's middleware or installed apps settings. In this case, CSP rules are by-spec not ordered. And sending duplicate values (what using sets aims to prevent) does have a real user impact due to wasting bandwidth.

Thanks for continuing to engage on this PR. Yes, I think I'm proposing Sequence[str], because it makes testing easier. Allowing set does avoid duplicates, perhaps leading to a better experience for django-csp users. It does add an additional burden on the django-csp devs, but that's how trade-offs work.

In Mozilla's bedrock, a set is used to ensure there are no duplicates, and then converted to a list to make django-csp happy:

https://github.com/mozilla/bedrock/blob/a092ffcd9d17c641cac1426fa4b1f9f7dae06f07/bedrock/settings/__init__.py#L87-L96

In Mozilla's fx-private-relay (where I'm a maintainer), a lot of code is used to append new items to lists and avoid duplicates:

https://github.com/mozilla/fx-private-relay/blob/a76e8c37ad19ef7e8d9133cdd7a0ccacfd1d1d15/privaterelay/settings.py#L109-L227

Using a set signals that the developer doesn't care about order. However, the tests do care, since we don't parse the generated header but just do a string compare. My suggestion is to convert any set to an alpha-sorted list, to enforce a consistent and testable order, and combine better with sequences. This would involve some more code in build_policy in csp/utils.py, something like:

        if v is not None:
            v = copy.copy(v)
            if isinstance(v, set):
                v = sorted(v)
            if not isinstance(v, (list, tuple)):
                v = (v,)
            csp[k] = v

This would be needed on both the config and update passes.

This way, we've got all ordered sequences when we get down to the end of build_policy. You could probably remove the line with dict.fromkeys to preserve order, and avoid the subtle change in Python 3.13.

@crgwbr crgwbr force-pushed the cweber/support-sets branch from b55d28f to 7a50495 Compare January 10, 2025 17:28
@crgwbr
Copy link
Contributor Author

crgwbr commented Jan 10, 2025

Ok, I've incorporated some of those notes, but kept the duduplication logic at the end. That prevents duplicate values from creeping in if the same value is supplied twice, once by normal settings and once by an update decorator.

@jwhitlock
Copy link
Member

Are you able to see details of failing tests? If not, I can copy them.

Fixes several exceptions that'd be thrown when trying to use Python sets as
config values, rather than tuples or lists.
@crgwbr
Copy link
Contributor Author

crgwbr commented Jan 11, 2025

Are you able to see details of failing tests? If not, I can copy them.

I can see them, but seems like an erroneous failure. The error is due to ruff formatting:

csp/tests/test_templatetags.py::ruff::format Would reformat: csp/tests/test_templatetags.py
FAILED

But, I didn't change that file. When I try to reformat it by running using the version of ruff I already had installed, there was no change to the file. I had to update ruff from 0.8 -> 0.9 before it would change anything. IOW, it's only failing due to the ruff version not being pinned. Regardless, I'll push a fix in a couple minutes.

@crgwbr crgwbr force-pushed the cweber/support-sets branch from 7a50495 to 6ff5146 Compare January 11, 2025 19:10
Copy link
Member

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @crgwbr. I think this is ready to merge, but I would prefer preserving order for the generated lists. I know devs like you using set don't care about the order, but others using list and tuple expect the header to be in declaration order. If you want to make the suggested change, as well as the test updates, I'd appreciate it, but I could also make that change in my own PR. Let me know which way you'd like to go.

@crgwbr crgwbr force-pushed the cweber/support-sets branch from 6ff5146 to 55e3ea0 Compare January 14, 2025 23:36
Copy link
Member

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks great. Thanks @crgwbr for sticking with the review process and being so responsive!

@jwhitlock jwhitlock merged commit 7a8a44d into mozilla:main Jan 15, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants